-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Python Source Distribution (sdist) support #457
base: development
Are you sure you want to change the base?
Conversation
c459d59
to
6afa443
Compare
Rebased on main. This is more a discussion of "should we have this" instead of "we should have this" PR, I'm just bringing the prototype code/changes that can achieve it ^^ |
Roughly, you can use the following lines to test this; pip install build
pushd Core/ClientSMLSWIG/Python
# -f to ignore if it doesnt exist
rm -f dist/*
pyproject-build . -vvvv -s
pip install dist/* -vvvv
popd |
Well, first what are the additional platforms that could be supported with this that might be interesting?
|
FTR, we could support most of those with wheel distributions. Other platforms I could add to that list would be AIX / z/OS / freeBSD, etc. I'd argue RISC-V would become more of an interest in the future ^^ WASM would be outside scope for python source packaging, imo that one could be better served with some rust-esc interface that people can then build upon and compile for WASM. We could possibly attach this file to github releases, to provide people a way to install directly? Though, that would be almost indistinguishable from a full clone + local build.
Cross-compilation would be better done within CI and specialised environments, I don't know if this method would allow anyone to more effectively and easier re-package Soar, but since its a build method limited to python distribution, I doubt it. I'll have to be honest, like this, it feels like this usecase is a solution looking for a problem. It is nice to define the sdist method, so people can be expected that it works, but since we might have a problem with releasing it as-is (because we want to prevent build errors on users' systems, and instead CI it for as many systems as possible), it might not be too useful. Yeah, unless we want to make soar-sml buildable on systems, there are better ways to do this (such as the install-via-github command), and the only other use-case for this would be someone packaging soar-sml for an offline machine. With that, I think adding the machinery for an sdist would maybe be useful for developers and researchers if they need it, but it doesn't solve a concrete problem at the moment. |
# We cannot just add the directories, as they will then start to include the .tar.gz file, | ||
# which scons treats as a "target", creating a circular dependency. | ||
SOURCE_EXTS = {"h", "c", "cpp", "hpp", "cxx"} | ||
for i in range(0, 5): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that we'll ever go more than 5 deep, but if we did this would break and we might not notice. I would feel more comfortable with some logic that checks if any more files are being adde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could just bump it from 5 to 10, would that suffice?
And add a CI job that does a basic sdist packaging + sdist-based build-and-install, possibly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, 10 sounds fine. Testing in CI would also be a good idea.
It doesn't look like a huge piece of implementation, honestly, so I wouldn't mind just adding it. Soar is the type of thing that might get used on a weird platform. |
Let me then also add the appropriate documentation to explain such a use-case :) |
This PR depends on #448, and will remain a draft until it merges, and can be rebased.
This PR adds experimental "Source Distribution" (or;
sdist
) support for python packaging. Basically,sdist
distributions are all source files necessary to build a package, so that whenpip
can't find a pre-built binary, it will pull the sdist.tar.gz
file, and attempt to build it itself.As you can guess, this is prone to failure, and is only meant as a last-resort usecase, the CI conditions in #448 aren't present on all machines that Soar would want to be installed on, nor have all edgecases been enumerated in the
SConstruct
file, to support building on a variety of targets.Yet, I wanted to try, and this PR - at least experimentally - allows for the creation of an
sdist
.tar.gz
file when ran withpython -m build Core/ClientSMLSWIG/Python --sdist
.This file can then be uploaded to pypi alongside the binary
.whl
distributions, or installed locally withpip install soar_sml-[...].tar.gz
There is an argument to be had about "Should we even have this? We don't support every single system that's out there" and that's a fair point, which is why I separated this from #448, to talk/discuss that, and also to extend the invitation of tons of testing on all sorts of systems.
If
sdist
files are uploaded for any version ofsoar-sml
,pip
will attempt to build it, when it has no binary release to grab.This may result in errors (as the system does not have the right build tools, or other conditions preventing its success), and may result in extra support requests (or other similar issues).